This repository was archived by the owner on Apr 14, 2022. It is now read-only.
Fix caching of compiled modules, analysis hang #1737
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ts, do not treat all compiled modules as builtins
Member
Author
|
This will conflict with #1686, and after it and this are merged the DB version will have to be upped (since the most recent changes are pretty impactful). |
Member
Author
|
The reason for the hang is the unhandled exception I mention above. The crash happens when the parsing task in |
jakebailey
commented
Nov 1, 2019
src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs
Outdated
Show resolved
Hide resolved
Member
Author
|
Going to merge #1686 first and then fix up this PR to match the new calls. Should be easier than trying to solve the conflicts the other way around. |
Member
Author
|
This isn't playing nice with #1686 merged. With |
Member
Author
|
Bumping the version stops the looping forever (some bad state? but I deleted my stored analysis...), but: The ending of: Is nowhere near what it was before #1686. |
|
Frankly I would switch off caching for now as it is changing. Loop resolution fix is not completely tested with caching either. But up to you. |
Member
Author
|
I'll probably have to disable it, though I think this PR needs to be going in anyway (since I think the existing behavior produces weird results and I'd rather not leave partially broken code around when I have a fix). |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1601.
This fixes a number of things which when combined broke caching of compiled builtins on non-Windows platforms.
On lookup, the module type was hardcoded to "specialized". The module type is involved in calculating the unique name for a module, so depending on what module is being looked up, the wrong ID would be calculated. (Worse, an unexpected code path would be taken for a module, causing unhandled exceptions, something to work on later.) Now, pass the module type down when looking up a module so that the unique ID is always calculated with the same inputs.
IsCompiledis no longer used for checking builtins when calculating the unique name, justModuleType.CompiledBuiltin. This would cause issues for top-level compiled files that weren't actually a part of the distribution.The module cache no longer holds a lock when dealing with dependencies. The lock could have possibly caused a deadlock (partially fixed in #1713), and I do not believe that it is required because of the analysis ordering.
Why did this affect "compiled builtins"? Compiled builtins (like
_functoolsor_weakref) are special in that they do not have any on-disk location. We must construct a fake filename in order to use later for producing scraper output file names and other calculation. Some checks rely on a file path and have to special case these compiled builtins, which means there's more bug surface.